Skip to content

Conversation

BengangY
Copy link
Contributor

@BengangY BengangY commented Aug 20, 2025

Implement dynamic control of firewalld service.
The first PR defines dynamic firewall control function, variable, and the http service's dynamic control.
The other services's dynamic control will be in the later PR.

@BengangY BengangY changed the title CP-308800: Dynamic control firewalld service (1) CP-308800: Dynamic control firewalld service - part 1 Aug 20, 2025
@BengangY BengangY changed the title CP-308800: Dynamic control firewalld service - part 1 CP-308800: Dynamic control of firewalld service - part 1 Aug 20, 2025
@BengangY BengangY marked this pull request as ready for review August 20, 2025 02:52
@BengangY BengangY force-pushed the private/bengangy/CP-308800 branch 3 times, most recently from 46f6446 to 6fbb10e Compare August 20, 2025 07:20
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this idea, it's similar to what I had in mind to remake the port management feature for the SDN, which seems very easy to add with this PR. I think it can be made even better to reduce complexity in the users' side

@BengangY BengangY marked this pull request as draft August 20, 2025 10:19
@BengangY BengangY force-pushed the private/bengangy/CP-308800 branch from 6fbb10e to ad68294 Compare August 21, 2025 11:11
@BengangY BengangY requested a review from psafont August 21, 2025 12:49
Comment on lines 91 to 96
| Xapi_insecure ->
("80", "TCP")
| _ ->
failwith
"service_type_to_port_and_protocol: Unsupported service type for \
iptables"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be moved to line 48, and the other service can be implemented, it shouldn't be difficult to look up the port and protocol.

Doesn't SSH auto mode need this to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other services will be implemented in later PRs.
For SSH, I'm considering if we should implement dynamic firewall control in xapi, as SSH service can be changed not only by xapi, but also by xapi-monitor-ssh, and systemd command directly.
Maybe we can just simply leave it opening.
Another option is to add the firewall-cmd to ExecStartPre and ExecStopPost of sshd systemd definition file.
What's your thought? @psafont

Copy link
Member

@psafont psafont Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For SSH, I'm considering if we should implement dynamic firewall control in xapi, as SSH service can be changed not only by xapi, but also by xapi-monitor-ssh, and systemd command directly.

xapi-monitor-ssh needs to work even if xapi is not working, but there are rpc functions exposed in xapi that need to do the same. I think it makes sense that xapi uses this interface to open and close the SSH port; and ideally xapi-monitor-ssh would be done in ocaml and use the same library, otherwise it will need to duplicate the functionality somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xapi-monitor-ssh is written in Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the easiest way to dynamic update sshd port is to add ExecStartPre and ExecStopPost to systemd definition file, as both xapi and ssh-monitor-ssh calls systemd to manage sshd. This can avoid the duplication.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the exact requirements from xenserver, but as far as I'm aware:

  • Switching of and on a single system instead of 2 means there's less chance of getting recovery and updates wrong. This means that I prefer a solution that blocks the port to the solution that blocks the ports and starts and stops the ssh daemon.
  • Sharing a module of code to open a close ports between different daemons means that it only needs one set of sets instead.
  • Modifying upstream service files and code increases maintenance costs, and as such I would avoid using ExecStartPre and ExecStopPost

So my preference would be to use this module both on xapi and xapi-monitor-ssh as the single mechanism to interrupt and enable access to SSH. But as I said previously, this is without knowing xenserver's feature requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requirement comes from engineers instead of customers. It's to avoid the difference between a port being firewalled and not having a listening service when doing the port scan.
xapi-monitor-ssh is written by Python. I don't think we should rewrite it with Ocaml to use the same firewall module (or do you mean call this module in the Python code?). If we have to implement it in both xapi and xapi-monitor-ssh, I think we can just implement it in the existing Python code.

@psafont
Copy link
Member

psafont commented Aug 21, 2025

Only the 2 last comments are important, the others are cosmetic

@BengangY BengangY force-pushed the private/bengangy/CP-308800 branch 2 times, most recently from cb836e3 to bed29a2 Compare August 22, 2025 07:19
@BengangY BengangY requested a review from psafont August 22, 2025 07:26
@BengangY BengangY force-pushed the private/bengangy/CP-308800 branch from bed29a2 to 4479248 Compare August 22, 2025 07:32
@BengangY BengangY marked this pull request as ready for review August 22, 2025 08:32
@BengangY BengangY force-pushed the private/bengangy/CP-308800 branch from 4479248 to 3183690 Compare August 22, 2025 10:33
@BengangY BengangY force-pushed the private/bengangy/CP-308800 branch 2 times, most recently from 1bedacb to 58c2b22 Compare August 25, 2025 05:37
@BengangY BengangY requested a review from psafont August 25, 2025 07:49
@BengangY BengangY force-pushed the private/bengangy/CP-308800 branch from 58c2b22 to e682b80 Compare August 26, 2025 10:26
@BengangY BengangY merged commit 4c9f4a4 into xapi-project:feature/dynamic-firewalld-control Aug 26, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants